fix(runtime): avoid implicit computer-use images - #3585

Reviewed at exact head 99c27eb2be4563eefa6ccbe7d1d369d9488d2bcb.

Coverage: what decides whether a screenshot reaches the model, which paths this change moves from implicit-attach to no-attach, and whether anything that should still get a frame loses one. Not covered: wording.

The intent is right and the observable behaviour matches it: the decision is per tool call, not per turn or session; observe: true, screenshot, zoom, and the legacy coordinate actions still carry an image, while other semantic actions keep a fresh accessibility observation without the frame. Two problems in how that decision is made.

[P1] The image gate reads mutable caller input, not the frozen call it executed

packages/runtime/src/computer-use-tools.ts:2818-2832 — toModelOutput: ({ input, output }) calls shouldSendScreenshotToModel(input), and that input is the original caller-held object at settlement time, not the immutable snapshot the implementation actually ran with.

Reproduced: begin an async settlement with observe / include_screenshot: false; after impl has started, mutate the original object to true; the returned result carries the PiP screenshot and the provider modelOutput then contains a file block. The reverse mutation swallows an image the caller explicitly asked for. The PiP frame a semantic action carries can be reclassified the same way before settlement — which walks straight through the boundary this PR exists to establish.

Cloning inside the callback would already be too late. The fix is for toModelOutput to use the same verified immutable call, or for impl to emit an explicit model-image decision derived from the frozen input.

[P2] The action catalogue is untyped and parallel to the canonical union

computer-use-tools.ts:345-367 introduces MODEL_SCREENSHOT_ACTIONS as a ReadonlySet<string> sitting alongside CU_ACTION_TYPES / CU_TOOL_ACTION_TYPES, and the tests sample only five inputs.

The consequence is a silent default: a legitimately added action will simply not send an image, while every existing schema and parity test still passes. That is precisely the drift the comments near core warn about. Deriving the set exhaustively from the canonical action union — a typed map or derived set — and iterating every action in a classification test would make a new action a compile-time or test-time decision instead of a silent one.

Verification and limits

Built clean in dependency order (core → storage → mcp → runtime). Computer Use suites 261/261; Biome and git diff --check pass. Across the whole Runtime: 3064 pass, 1 fail, 13 skipped — the single failure is a macOS Bash executable-root environment assertion in untouched code, reproducible standalone, and hosted CI is green on this exact head.

Incremental closure check at exact head 99c27eb2be4563eefa6ccbe7d1d369d9488d2bcb: the head has not changed since review 5002157364, so both previously reported blockers remain. Reposting them inline at their exact diff authorities; no APPROVE on this head.

Thanks — narrowing which Computer Use results carry an image into the model is worth doing, and test is green on e7205fa5.

I re-checked the two earlier points against this head. Both still apply; details are inline.

The behaviour change is right and the test covers the interesting cases — semantic actions and observe without a picture stay text-only, while screenshot, an explicit include_screenshot, and the coordinate actions still get the image block.

One thing to fix before this merges.

[P2] MODEL_SCREENSHOT_ACTIONS restates CU_ACTION_TYPES by hand, and nothing holds the two together

The new set in packages/runtime/src/computer-use-tools.ts:345-360 is exactly CU_ACTION_TYPES minus cursor_position and wait. It is written out as fifteen string literals with no link back to the source list and no test comparing them.

That is the drift this area has already paid for once. packages/core/src/computer-use.ts:227-251 documents it directly: a hand-written list beside a schema that listed the same names fell behind when window_action was added, and the fix was to stop hand-writing it and add computer-use-schema-parity.test.ts to compare the lists in both directions. The comment also makes the point that matters here — drift where a name is missing is "just as costly and quieter."

The failure mode is worse than the original one, because it is silent in both directions. Add a coordinate action and it reaches the model with no picture, which looks like a model that cannot see rather than a missing list entry; no test fails. Remove or rename one and the dead entry never announces itself.

Deriving the set removes the class of bug rather than the instance:

const NON_VISUAL_COORDINATE_ACTIONS = new Set(['cursor_position', 'wait']);
const MODEL_SCREENSHOT_ACTIONS: ReadonlySet<string> = new Set(
  CU_ACTION_TYPES.filter((action) => !NON_VISUAL_COORDINATE_ACTIONS.has(action)),
);

Now adding a coordinate action opts it into vision by default, and deliberately withholding a picture is a one-line, reviewable edit to the exclusion set. If you would rather keep the explicit list, please extend computer-use-schema-parity.test.ts to assert this set against CU_ACTION_TYPES so drift fails a test instead of quietly blinding the model.

const NON_VISUAL_COORDINATE_ACTIONS = new Set(['cursor_position', 'wait']);
const MODEL_SCREENSHOT_ACTIONS: ReadonlySet<string> = new Set(
  CU_ACTION_TYPES.filter((action) => !NON_VISUAL_COORDINATE_ACTIONS.has(action)),
);

The rewrite is better than what I asked for. Turning the set into COMPUTER_USE_MODEL_SCREENSHOT_POLICY keyed by every action, with satisfies Record<CuToolActionType, …>, makes the compiler reject a new action that nobody classified — that is stronger than the parity test I suggested, because it fails at build time rather than in a test run, and the added Object.keys(...) assertion still covers the ordering and the runtime shape. The three-way always / explicit / never split also reads better than the implicit "everything except two" it replaces.

Binding the decision to includeScreenshotInModelOutput on the executed result, instead of re-reading input in toModelOutput, closes a hole I had not raised: the input object is reachable and mutable after the call, so the old form let a later mutation change what the model was shown for an invocation that had already run. The regression test for exactly that is a good addition.

test is green on the current head and nothing is outstanding on my side. Approving.